Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrectly transposed matrix for displayp3 colorspaces #1960

Conversation

ld-kerley
Copy link
Contributor

When originally authored the matrix was authored transposed. Further internal testing revealed this to manifest as an undesired shift towards red.

Before

mtlx_p3_to_rec709_before

After

mtlx_p3_to_rec709_fixed

@jstone-lucasfilm
Copy link
Member

Great catch, thanks @ld-kerley! I'm CC'ing @doug-walker and @carolalynn on this PR, to make sure we're remaining aligned with the upcoming NanoColor project.

@dgovil
Copy link
Contributor

dgovil commented Aug 6, 2024

Also just cc'ing @meshula about this too.

@doug-walker
Copy link

doug-walker commented Aug 7, 2024

@jstone-lucasfilm , it looks like this PR should get MaterialX much closer to NanoColor.

Here is what NanoColor gives for that conversion, based on the draft config that implements the ASWF Color Interop Forum color spaces and evaluating the result at single precision:
1.224940, -0.04205696, -0.01963755,
-0.2249402, 1.042057, -0.07863604,
0.0, 0.0, 1.098274

So it's fairly well aligned with this PR (enough to normally be below visual tolerances) but there are typically some small differences in matrices such as this based on the details of how they are calculated. For OCIO we use the chromaticity coordinates as the definitive parameters and calculate the matrix values to the reference space at double precision. But then since neither lin_displayp3_scene or lin_rec709_scene are the reference space, there would be some slight loss involved when composing the two matrices (although this is done at double precision too).

The direct double precision result from chromaticities, without compositional loss, is the following (which would be closest to the "true" value):
1.2249401763e+00, -4.2056954710e-02, -1.9637554590e-02,
-2.2494017628e-01, 1.0420569547e+00, -7.8636045551e-02,
0.0, 0.0, 1.0982736001e+00

@jstone-lucasfilm
Copy link
Member

Thanks @doug-walker, and in this context, I'd have no objections to moving forward with the matrix transpose in this pull request. Further in the future, we're planning to import our color transform graphs directly from NanoColor, at which point any differences in precision assumptions should be resolved.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks @ld-kerley!

@jstone-lucasfilm jstone-lucasfilm merged commit a20bef0 into AcademySoftwareFoundation:main Aug 7, 2024
34 checks passed
@meshula
Copy link

meshula commented Aug 8, 2024

Since @dgovil summoned me, the matrix Doug wrote out matches the results of the nanocolor implementation currently underpinning GfColor in OpenUSD.

jstone-lucasfilm pushed a commit that referenced this pull request Nov 5, 2024
Backporting #1960 to legacy 1.38 branch.

This feels like a pretty easy bug fix backport that would be to everyones benefit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants